Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Catalog image lazy load #27033

Merged
merged 6 commits into from
Mar 2, 2020
Merged

Catalog image lazy load #27033

merged 6 commits into from
Mar 2, 2020

Conversation

tdgroot
Copy link
Member

@tdgroot tdgroot commented Feb 26, 2020 •
edited
Loading

Description (*)

This changeset adds lazy loading to the catalog product list.

Fixed Issues (if relevant)

  1. Add image lazy loading #27032: Add image lazy loading

Manual testing scenarios (*)

  1. Have a product list with more than 16 products, all having unique products.
  2. Check inspector if images are loaded through scrolling.

Related pull requests

Questions or comments

I think discussions on the topic 'image lazy loading' should be held on #27032.

Any code reviewing comments can be placed in this PR.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Feb 26, 2020

Hi @tdgroot. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@tdgroot tdgroot linked an issue Feb 26, 2020 that may be closed by this pull request
Add image lazy loading #27032
Closed
@ihor-sviziev ihor-sviziev self-assigned this Feb 27, 2020
ihor-sviziev
ihor-sviziev requested changes Feb 27, 2020
View reviewed changes
app/code/Magento/Catalog/view/frontend/templates/product/image_with_borders.phtml Outdated
<?= $escaper->escapeHtmlAttr($block->getCustomAttributes()) ?>
src="<?= $escaper->escapeUrl($block->getImageUrl()) ?>"
loading="lazy"
width="<?= $escaper->escapeHtmlAttr($block->getWidth()) ?>"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why you changed max-width and max-height to just width and height? It looks not related to this PR.

As max width and max height are not valid attributes - seems like we could just remove them

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The width and height attributes are needed to preserve the aspect ratio of the image when it is not loaded yet.

Copy link
Contributor

@ihor-sviziev ihor-sviziev Feb 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, no problem, but need just test it carefully on different devices, because it could do regression

app/code/Magento/Catalog/view/frontend/templates/product/image.phtml Outdated Show resolved Hide resolved
app/code/Magento/Catalog/view/frontend/templates/product/image_with_borders.phtml Outdated Show resolved Hide resolved
@engcom-Echo engcom-Echo self-assigned this Feb 28, 2020
@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Feb 28, 2020
@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests B2B

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7006 has been created to process this Pull Request

@ihor-sviziev ihor-sviziev mentioned this pull request Feb 28, 2020
Closed
4 tasks
@engcom-Alfa engcom-Alfa self-assigned this Mar 2, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@magento-engcom-team magento-engcom-team mentioned this pull request Mar 2, 2020
Closed
magento-engcom-team pushed a commit that referenced this pull request Mar 2, 2020
@VladimirZaets
ENGCOM-7006: Catalog image lazy load #27033
56b0353
@magento-engcom-team magento-engcom-team merged commit 2a3c03f into magento:2.4-develop Mar 2, 2020
@m2-assistant
Copy link

m2-assistant bot commented Mar 2, 2020

Hi @tdgroot, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@sdzhepa sdzhepa mentioned this pull request May 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

Assignees

@ihor-sviziev ihor-sviziev

@engcom-Alfa engcom-Alfa

@engcom-Echo engcom-Echo

Labels
Area: Frontend Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Catalog Progress: accept Release Line: 2.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add image lazy loading
5 participants
@tdgroot @ihor-sviziev @magento-engcom-team @engcom-Alfa @engcom-Echo